Automated Test: large-image-processing #314
Conversation
📝 WalkthroughWalkthroughThis change implements a dual approach to upload size management: hardcoding a 10 MB client-side validation limit in JavaScript, fixing the server-side download cap at 10 MB for URL-based uploads, and introducing automatic server-side image downsampling to fit images within configured limits. The OptimizedImage API is refactored to accept dimensions as a single string parameter rather than separate width/height arguments. Changes
Sequence DiagramsequenceDiagram
actor Client
participant JS as JavaScript<br/>(utilities.js)
participant Controller as Controller<br/>(uploads_controller.rb)
participant ImageProcessor as OptimizedImage<br/>(optimized_image.rb)
participant Storage as File Storage
Client->>JS: Upload via URL
JS->>JS: Validate file size (10 MB limit)
alt Size exceeds 10 MB
JS->>Client: Show error
else Size OK
JS->>Controller: POST upload request
Controller->>Controller: Download file (10 MB cap)
alt File is image & size > max_image_size_kb
Controller->>ImageProcessor: Call downsize(from, to, dimensions)
ImageProcessor->>ImageProcessor: Optimize with 80% quality (up to 5 attempts)
ImageProcessor->>Controller: Return downsized image
end
Controller->>Storage: Persist upload
Controller->>Client: Success response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/controllers/uploads_controller.rb`:
- Line 72: The code passes tempfile.size into Upload.create_for which can be
stale if the downsizing loop rewrote the tempfile via the convert subprocess;
after the downsizing completes, recompute the actual on-disk size and pass that
to Upload.create_for (e.g. call File.size(tempfile.path) or equivalent) instead
of using tempfile.size so Upload.create_for receives the correct file size.
- Around line 63-70: The downsizing loop ignores OptimizedImage.downsize's
boolean return and uses tempfile.size (which may be stale); update the while
loop in the uploads controller to check the return value of
OptimizedImage.downsize and break on false to avoid infinite retries, and
replace usages of tempfile.size with File.size(tempfile.path) when comparing to
SiteSetting.max_image_size_kb.kilobytes so the size is re-statted after the
external conversion; keep the existing attempt retry counter but stop looping
immediately on a failed downsize to surface the error.
In `@app/models/optimized_image.rb`:
- Around line 145-151: Remove the duplicate 4-argument definition of
self.downsize in the OptimizedImage class (the overload that accepts from, to,
max_width, max_height) so only the canonical signature self.downsize(from, to,
dimensions, opts={}) remains, and update the call site in the ResizeEmoji job
that calls OptimizedImage.downsize(path, path, 100, 100, opts) to pass the
dimensions as a single string like "100x100" (OptimizedImage.downsize(path,
path, "100x100", opts)) so arguments bind correctly to dimensions and opts.
🧹 Nitpick comments (4)
app/assets/javascripts/discourse/lib/utilities.js (2)
182-182: Hardcoded magic number duplicated across two locations.
10 * 1024appears on both Line 182 and Line 246. Extract it to a named constant at the top of the object (e.g.,MAX_UPLOAD_SIZE_KB: 10 * 1024) and reference it in both places to keep the limit in sync and self-documenting.Also note: this change removes the per-type size distinction (image vs. attachment) that was previously driven by
Discourse.SiteSettings['max_' + type + '_size_kb']. If that's intentional (as the PR suggests), consider leaving a brief comment explaining why the dynamic setting is no longer consulted, to prevent a future contributor from thinking it's a mistake.
244-248: Wrapcase 413body in a block to avoid leaking the declaration to other clauses.Biome correctly flags that a
vardeclaration inside aswitchclause without braces is accessible from sibling clauses. Whilevaris function-scoped in JS and won't cause a runtime error here, wrapping the case body in a block improves clarity and prevents accidental access from fall-through cases.🔧 Proposed fix
// entity too large, usually returned from the web server - case 413: - var maxSizeKB = 10 * 1024; // 10 MB - bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB })); - return; + case 413: { + var maxSizeKB = 10 * 1024; // 10 MB + bootbox.alert(I18n.t('post.errors.file_too_large', { max_size_kb: maxSizeKB })); + return; + }app/controllers/uploads_controller.rb (2)
66-67: In-place conversion (from == to) is fragile.Passing the same path as both source and destination to ImageMagick's
convertrelies on the tool fully reading the input into memory before writing. This generally works but can corrupt the file with very large images or certain formats. Consider writing to a second temp file and then renaming, or verify that this pattern is safe for the image sizes expected (up to 10 MB).
55-55: Same hardcoded10.megabytesas the JS side — consider extracting a shared constant.This repeats the magic number from the JavaScript file. If the limit ever changes, all three locations (JS ×2, Ruby ×1) must be updated in lockstep. A server-side constant (e.g.,
MAX_UPLOAD_DOWNLOAD_SIZE = 10.megabytes) referenced here and exposed to JS would be more maintainable.
| # allow users to upload large images that will be automatically reduced to allowed size | ||
| if tempfile && tempfile.size > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename) | ||
| attempt = 5 | ||
| while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes | ||
| OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails) | ||
| attempt -= 1 | ||
| end | ||
| end |
There was a problem hiding this comment.
Downsizing loop ignores downsize return value; loop may spin on a failed conversion.
OptimizedImage.downsize returns false when the conversion command fails, but the return value is never checked. On failure the file is unchanged (or possibly corrupted), so subsequent iterations repeat the same failing command. Break on failure to avoid wasted work and to surface the error.
Additionally, tempfile.size on Line 66 may return a stale value after an external process (convert) rewrites the file — Ruby's IO#size uses fstat on the original file descriptor. Use File.size(tempfile.path) to stat the path directly.
🐛 Proposed fix
# allow users to upload large images that will be automatically reduced to allowed size
- if tempfile && tempfile.size > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename)
+ if tempfile && File.size(tempfile.path) > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename)
attempt = 5
- while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes
- OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
+ while attempt > 0 && File.size(tempfile.path) > SiteSetting.max_image_size_kb.kilobytes
+ unless OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails)
+ Rails.logger.error("Failed to downsize image: #{filename}")
+ break
+ end
attempt -= 1
end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # allow users to upload large images that will be automatically reduced to allowed size | |
| if tempfile && tempfile.size > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename) | |
| attempt = 5 | |
| while attempt > 0 && tempfile.size > SiteSetting.max_image_size_kb.kilobytes | |
| OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails) | |
| attempt -= 1 | |
| end | |
| end | |
| # allow users to upload large images that will be automatically reduced to allowed size | |
| if tempfile && File.size(tempfile.path) > 0 && SiteSetting.max_image_size_kb > 0 && FileHelper.is_image?(filename) | |
| attempt = 5 | |
| while attempt > 0 && File.size(tempfile.path) > SiteSetting.max_image_size_kb.kilobytes | |
| unless OptimizedImage.downsize(tempfile.path, tempfile.path, "80%", allow_animation: SiteSetting.allow_animated_thumbnails) | |
| Rails.logger.error("Failed to downsize image: #{filename}") | |
| break | |
| end | |
| attempt -= 1 | |
| end | |
| end |
🤖 Prompt for AI Agents
In `@app/controllers/uploads_controller.rb` around lines 63 - 70, The downsizing
loop ignores OptimizedImage.downsize's boolean return and uses tempfile.size
(which may be stale); update the while loop in the uploads controller to check
the return value of OptimizedImage.downsize and break on false to avoid infinite
retries, and replace usages of tempfile.size with File.size(tempfile.path) when
comparing to SiteSetting.max_image_size_kb.kilobytes so the size is re-statted
after the external conversion; keep the existing attempt retry counter but stop
looping immediately on a failed downsize to surface the error.
| attempt -= 1 | ||
| end | ||
| end | ||
|
|
||
| upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type, image_type: type) |
There was a problem hiding this comment.
tempfile.size passed to Upload.create_for may be stale after downsizing.
After the downsizing loop, tempfile.size is passed as the file size to Upload.create_for. If the file was rewritten by the convert subprocess, the IO-level size may not reflect the new on-disk size. Use File.size(tempfile.path) here as well for consistency.
🐛 Proposed fix
- upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type, image_type: type)
+ upload = Upload.create_for(current_user.id, tempfile, filename, File.size(tempfile.path), content_type: content_type, image_type: type)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| upload = Upload.create_for(current_user.id, tempfile, filename, tempfile.size, content_type: content_type, image_type: type) | |
| upload = Upload.create_for(current_user.id, tempfile, filename, File.size(tempfile.path), content_type: content_type, image_type: type) |
🤖 Prompt for AI Agents
In `@app/controllers/uploads_controller.rb` at line 72, The code passes
tempfile.size into Upload.create_for which can be stale if the downsizing loop
rewrote the tempfile via the convert subprocess; after the downsizing completes,
recompute the actual on-disk size and pass that to Upload.create_for (e.g. call
File.size(tempfile.path) or equivalent) instead of using tempfile.size so
Upload.create_for receives the correct file size.
| def self.downsize(from, to, max_width, max_height, opts={}) | ||
| optimize("downsize", from, to, max_width, max_height, opts) | ||
| optimize("downsize", from, to, "#{max_width}x#{max_height}", opts) | ||
| end | ||
|
|
||
| def self.optimize(operation, from, to, width, height, opts={}) | ||
| dim = dimensions(width, height) | ||
| def self.downsize(from, to, dimensions, opts={}) | ||
| optimize("downsize", from, to, dimensions, opts) | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n app/models/optimized_image.rb | sed -n '140,160p'Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 955
🏁 Script executed:
rg -n --type=ruby 'downsize\(' -A 1 -B 1Repository: admin-coderabbit/coderabbit-discourse-mirror
Length of output: 1110
Duplicate downsize definition — the 4-arg overload on line 145 is dead code and will break existing callers.
Ruby does not support method overloading. The second def self.downsize (line 149) silently replaces the first (line 145), making the 4-arg version unreachable. However, this breaks the existing caller in app/jobs/regular/resize_emoji.rb:14, which uses the old signature: OptimizedImage.downsize(path, path, 100, 100, opts).
At runtime, the 4-arg call will mis-bind to the 3-arg signature: the two integers (100, 100) will land in dimensions and opts respectively, causing incorrect behavior or runtime errors when passed to optimize.
Remove the old 4-arg definition on lines 145–147. Update the resize_emoji caller to pass dimensions as a string: OptimizedImage.downsize(path, path, "100x100", opts).
🐛 Proposed fix — remove the dead overload and update the caller
app/models/optimized_image.rb:
- def self.downsize(from, to, max_width, max_height, opts={})
- optimize("downsize", from, to, "#{max_width}x#{max_height}", opts)
- end
-
def self.downsize(from, to, dimensions, opts={})
optimize("downsize", from, to, dimensions, opts)
endapp/jobs/regular/resize_emoji.rb:
- OptimizedImage.downsize(path, path, 100, 100, opts)
+ OptimizedImage.downsize(path, path, "100x100", opts)🧰 Tools
🪛 RuboCop (1.84.0)
[warning] 149-149: Method OptimizedImage.downsize is defined at both app/models/optimized_image.rb:145 and app/models/optimized_image.rb:149.
(Lint/DuplicateMethods)
🤖 Prompt for AI Agents
In `@app/models/optimized_image.rb` around lines 145 - 151, Remove the duplicate
4-argument definition of self.downsize in the OptimizedImage class (the overload
that accepts from, to, max_width, max_height) so only the canonical signature
self.downsize(from, to, dimensions, opts={}) remains, and update the call site
in the ResizeEmoji job that calls OptimizedImage.downsize(path, path, 100, 100,
opts) to pass the dimensions as a single string like "100x100"
(OptimizedImage.downsize(path, path, "100x100", opts)) so arguments bind
correctly to dimensions and opts.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit